gh-101773: Optimize creation of Fractions in private methods #101780

skirpichev · 2023-02-10T09:09:38Z

removed private _normalize argument of the Fraction constructor
added _from_coprime_ints() helper to make fractions from a pair of coprime ints

Issue: Optimize creation of unnormalized Fraction's in private methods (arithmetics, etc) #101773

Lib/fractions.py

skirpichev · 2023-02-11T01:06:10Z

Probably, from_float/decimal methods should be adapted to use this. Decimal.as_integer_ratio is in lowest terms. I guess, that same holds for the float method, but that's undocumented, unfortunately.

Lib/fractions.py

mdickinson · 2023-02-11T11:03:55Z

@skirpichev

I guess, that same holds for the float method, but that's undocumented, unfortunately

It does indeed. It would be reasonable to document it, I think.

skirpichev · 2023-02-11T13:24:49Z

On Sat, Feb 11, 2023 at 04:53:50AM -0800, Mark Dickinson wrote: I think you can and should assume that all core developers are very familiar with PEP 8. :-)

Here I do care more about users knowledge. The single leading underscore as a stigmate of private (internaly used) entity is more or less common convention across the python ecosystem. Sometimes it's enforced (e.g. help() hides such methods, this stuff is not included in star-imports and so on). I wonder if we shouldn't do same with _-prefixed kwargs (at least in help()).

That's a rather dogmatic approach. On CPython, we should prefer a more pragmatic approach: if we have reasonable grounds to believe that a change might break user code then we should take reasonable steps to avoid that breakage. It's not friendly to knowingly break existing code and then tell people "well, it was your fault for using something undocumented".

But the Python is a dynamical language and with great introspection capabilities... In this way we can end with supporting everything as part of the API, because people actually *can* use everything.

skirpichev · 2023-02-24T08:05:56Z

@mdickinson, _normalize part was reverted. Does it make sense now?

mdickinson · 2023-02-26T12:54:10Z

In this way we can end with supporting everything as part of the API.

Indeed. :-) As with most aspects of software development, careful judgement is required, evaluating the context and balancing the trade offs within that context. Few things are black and white. The fact that _normalize was intended to be private, and has a leading underscore, certainly means that removing it is easier than if it were public, but it's still something that shouldn't be done without thinking it through first. Hyrum's law is a real thing.

@skirpichev

Does it make sense now?

Hmm, not really. :-) I don't want to leave a _normalize flag that's unused and untested, indefinitely. I think we have three options:

Leave this alone.
Introduce _from_pair (possibly after renaming - see below) and immediately remove the _normalize flag
Introduce _from_pair (ditto) and deprecate the _normalize flag.

Of those options, option 3 just seems like too much code churn for too little benefit.

There was a recent discuss.python.org thread in which the meaning of _normalize was misinterpreted, which pushes me a bit further towards option 2. Arguably, _normalize is too discoverable right now (since it shows up in IDE completions and tooltips), and there's a temptation to guess what it means. _from_pair would also be somewhat discoverable, but at least we get to add a docstring that clearly indicates that it's private, and the expected constraints for people who decide to use it anyway.

Lib/fractions.py

bedevere-bot · 2023-02-26T13:00:45Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

This reverts commit 4324391.

skirpichev · 2023-02-27T04:08:48Z

Arguably, _normalize is too discoverable right now (since it shows up in IDE completions and tooltips)

That is a more generic problem: we have PEP8's convention where _single_leading_underscore is weak “internal use” indicator. On another hand, these arguments are available for introspection and shown in the help() output, IDE tooltips and so on.

Probably, all above use the signature() function. Maybe it does make sense to add a private=False option for this function to hide private kwargs per default?

There was a recent discuss.python.org thread in which the meaning of _normalize was misinterpreted

(Probably it was about this thread.) Indeed, there was an impression that all methods do support unnormalized fractions.

offtopic

On another hand 1. there is no doubts that the argument was private 2. most methods actually do support this (arithmetics) and 3. much more can if we do a cheap normalization (ensure that denominator is positive)

So, I think the proposal might have some sense and it's possible to implement it with a little overhead for canonicalized fractions. E.g. we can mark unnormalized fractions with a sticky flag (or flags) and do special steps to deal with this case in some methods. Not sure it worth to have this in the stdlib: to really support unnormalized fractions almost every method should be rewritten (e.g. arithmetic methods do some normalization of the output, which we probably want to avoid for unnormalized fractions).

PS:
I have made the requested changes; please review again.

bedevere-bot · 2023-02-27T04:08:52Z

Thanks for making the requested changes!

@mdickinson: please review the changes made to this pull request.

eendebakpt · 2023-02-27T09:06:55Z

@skirpichev @mdickinson The changes look good to me. With the removal of the (semi-private) argument _normalize, we could update the whatsnew entry with a note stating that it is removed.

Misc/NEWS.d/next/Library/2023-02-10-11-59-13.gh-issue-101773.J_kI7y.rst

…_kI7y.rst Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>

mdickinson

LGTM. I tweaked the docstring wording on _from_coprime_ints slightly.

I'll merge when CI completes.

skirpichev · 2023-02-28T02:24:38Z

Thanks to all reviewers.

* main: (67 commits) pythongh-99108: Add missing md5/sha1 defines to Modules/Setup (python#102308) pythongh-100227: Move _str_replace_inf to PyInterpreterState (pythongh-102333) pythongh-100227: Move the dtoa State to PyInterpreterState (pythongh-102331) pythonGH-102305: Expand some macros in generated_cases.c.h (python#102309) Migrate to new PSF mailgun account (python#102284) pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (in Python/) (python#102193) pythonGH-90744: Fix erroneous doc links in the sys module (python#101319) pythongh-87092: Make jump target label equal to the offset of the target in the instructions sequence (python#102093) pythongh-101101: Unstable C API tier (PEP 689) (pythonGH-101102) IDLE: Simplify DynOptionsMenu __init__code (python#101371) pythongh-101561: Add typing.override decorator (python#101564) pythongh-101825: Clarify that as_integer_ratio() output is always normalized (python#101843) pythongh-101773: Optimize creation of Fractions in private methods (python#101780) pythongh-102251: Updates to test_imp Toward Fixing Some Refleaks (pythongh-102254) pythongh-102296 Document that inspect.Parameter kinds support ordering (pythonGH-102297) pythongh-102250: Fix double-decref in COMPARE_AND_BRANCH error case (pythonGH-102287) pythongh-101100: Fix sphinx warnings in `types` module (python#102274) pythongh-91038: Change default argument value to `False` instead of `0` (python#31621) pythongh-101765: unicodeobject: use Py_XDECREF correctly (python#102283) [doc] Improve grammar/fix missing word (pythonGH-102060) ...

…numerator/denominator pair is already normalised. Follows python/cpython#101780

https://build.opensuse.org/request/show/1073017 by user dgarcia + dimstar_suse - Enable python 3.11 build again, now is supported - Update to 1.14 - Implement __format__ for Fraction, following python/cpython#100161 - Implement Fraction.is_integer(), following python/cpython#100488 - Fraction.limit_denominator() is faster, following python/cpython#93730 - Internal creation of result Fractions is about 10% faster if the calculated numerator/denominator pair is already normalised, following python/cpython#101780 - Built using Cython 3.0.0b1. - 1.13 - Parsing very long numbers from a fraction string was very slow, even slower than fractions.Fraction. The parser is now faster in all cases (and still much faster for shorter numbers). - Fraction did not implement __int__. https://bugs.python.org/issue44547 - 1.12 - Faster and more spa

bedevere-bot added the awaiting review label Feb 10, 2023

bedevere-bot mentioned this pull request Feb 10, 2023

Optimize creation of unnormalized Fraction's in private methods (arithmetics, etc) #101773

Closed

AlexWaygood added the performance Performance or resource usage label Feb 10, 2023

pythongh-101773: Optimize creation of Fraction's in private methods

9cc62f7

skirpichev force-pushed the opt-fractions-new branch from 130db33 to 9cc62f7 Compare February 10, 2023 09:41

Merge branch 'main' into opt-fractions-new

836d3a6

eendebakpt reviewed Feb 10, 2023

View reviewed changes

Lib/fractions.py Show resolved Hide resolved

eendebakpt reviewed Feb 10, 2023

View reviewed changes

Lib/fractions.py Show resolved Hide resolved

eendebakpt reviewed Feb 10, 2023

View reviewed changes

Lib/fractions.py Outdated Show resolved Hide resolved

skirpichev requested a review from eendebakpt February 10, 2023 12:02

mdickinson reviewed Feb 11, 2023

View reviewed changes

Lib/fractions.py Outdated Show resolved Hide resolved

mdickinson reviewed Feb 11, 2023

View reviewed changes

Lib/fractions.py Show resolved Hide resolved

skirpichev mentioned this pull request Feb 11, 2023

as_integer_ratio docstring's #101825

Closed

skirpichev added 4 commits February 11, 2023 15:13

Adjust signature of _from_pair() and add docstring

3625d34

Use new helper in from_float/Decimal

9e66b5a

Restore back _normalize kwarg for the Fraction.__new__()

4324391

Amend 3625d34

b0dca38

mdickinson requested review from mdickinson and eendebakpt and removed request for eendebakpt February 12, 2023 11:02

eendebakpt approved these changes Feb 12, 2023

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Feb 12, 2023

merwok changed the title ~~gh-101773: Optimize creation of Fraction's in private methods~~ gh-101773: Optimize creation of Fractions in private methods Feb 19, 2023

mdickinson reviewed Feb 26, 2023

View reviewed changes

Lib/fractions.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting core review labels Feb 26, 2023

mdickinson self-assigned this Feb 26, 2023

skirpichev added 4 commits February 27, 2023 06:14

Nitpick on docstring (int's -> ints)

3a8506d

Revert "Restore back _normalize kwarg for the Fraction.__new__()"

13262af

This reverts commit 4324391.

Rename: _from_pair -> _from_coprime_ints

4e8977a

Merge branch 'main' into opt-fractions-new

57ac373

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 27, 2023

bedevere-bot requested a review from mdickinson February 27, 2023 04:08

eendebakpt reviewed Feb 27, 2023

View reviewed changes

Misc/NEWS.d/next/Library/2023-02-10-11-59-13.gh-issue-101773.J_kI7y.rst Outdated Show resolved Hide resolved

skirpichev and others added 2 commits February 27, 2023 12:24

Update Misc/NEWS.d/next/Library/2023-02-10-11-59-13.gh-issue-101773.J…

4b4a386

…_kI7y.rst Co-authored-by: Pieter Eendebak <pieter.eendebak@gmail.com>

+1

580b1be

skirpichev force-pushed the opt-fractions-new branch from 4339283 to 580b1be Compare February 27, 2023 09:38

Tweak docstring wording

53d5e94

mdickinson approved these changes Feb 27, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Feb 27, 2023

mdickinson merged commit 4f3786b into python:main Feb 27, 2023

bedevere-bot removed the awaiting merge label Feb 27, 2023

skirpichev deleted the opt-fractions-new branch February 28, 2023 02:23

scoder added a commit to scoder/quicktions that referenced this pull request Mar 19, 2023

Speed up the internal creation of result Fractions if the calculated …

c5b6a2e

…numerator/denominator pair is already normalised. Follows python/cpython#101780

hauntsaninja mentioned this pull request Apr 28, 2023

[NO ISSUE] Adds __eq__ support for non-normalized fractions #103959

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-101773: Optimize creation of Fractions in private methods #101780

gh-101773: Optimize creation of Fractions in private methods #101780

skirpichev commented Feb 10, 2023 •

edited

Loading

skirpichev commented Feb 11, 2023

mdickinson commented Feb 11, 2023

skirpichev commented Feb 11, 2023 via email

skirpichev commented Feb 24, 2023

mdickinson commented Feb 26, 2023 •

edited

Loading

bedevere-bot commented Feb 26, 2023

skirpichev commented Feb 27, 2023 •

edited

Loading

bedevere-bot commented Feb 27, 2023

eendebakpt commented Feb 27, 2023

mdickinson left a comment

skirpichev commented Feb 28, 2023

gh-101773: Optimize creation of Fractions in private methods #101780

gh-101773: Optimize creation of Fractions in private methods #101780

Conversation

skirpichev commented Feb 10, 2023 • edited Loading

skirpichev commented Feb 11, 2023

mdickinson commented Feb 11, 2023

skirpichev commented Feb 11, 2023 via email

skirpichev commented Feb 24, 2023

mdickinson commented Feb 26, 2023 • edited Loading

bedevere-bot commented Feb 26, 2023

skirpichev commented Feb 27, 2023 • edited Loading

bedevere-bot commented Feb 27, 2023

eendebakpt commented Feb 27, 2023

mdickinson left a comment

Choose a reason for hiding this comment

skirpichev commented Feb 28, 2023

skirpichev commented Feb 10, 2023 •

edited

Loading

mdickinson commented Feb 26, 2023 •

edited

Loading

skirpichev commented Feb 27, 2023 •

edited

Loading